GH-49452: [Python] Reintroduce docstring injection for stubfiles#49453
GH-49452: [Python] Reintroduce docstring injection for stubfiles#49453rok wants to merge 7 commits intoapache:mainfrom
Conversation
|
|
|
@raulcd this is ready for review |
2f0f841 to
f885111
Compare
|
@github-actions crossbow submit wheel*-cp313-* |
|
Revision: f885111 Submitted crossbow builds: ursacomputing/crossbow @ actions-c1678cd8dd |
d638b95 to
10509c4
Compare
|
@github-actions crossbow submit wheel*-cp313-* |
|
Revision: 10509c4 Submitted crossbow builds: ursacomputing/crossbow @ actions-f3f679ce77 |
10509c4 to
e4903eb
Compare
|
@github-actions crossbow submit wheel*-cp313-* |
|
Revision: e4903eb Submitted crossbow builds: ursacomputing/crossbow @ actions-a33b20ecd0 |
|
@raulcd the docstring presence check was apparently too strict. I think this is ready for review now. |
|
@raulcd any we could get this reviewed by end of week? |
| Populate pyarrow_pkg with source Python modules and installed binary artifacts | ||
| so that pyarrow can be imported from the parent directory of pyarrow_pkg. | ||
| """ | ||
| ext_suffix = sysconfig.get_config_var("EXT_SUFFIX") or ".so" |
There was a problem hiding this comment.
Is this EXT_SUFFIX env var necessary? doesn't seem to be used anywhere apart from this line
There was a problem hiding this comment.
It is used below as:
is_extension = ext_suffix in artifact.name or artifact.suffix == ".pyd"There was a problem hiding this comment.
Sorry, I meant whether it can be something different than the default "*.so". I'm not sure is necessary to be able to override as an env variable, what's the use case?
There was a problem hiding this comment.
As per 🤖 :
sysconfig.get_config_var("EXT_SUFFIX") returns the platform-specific extension suffix (e.g. .cpython-313-x86_64-linux-gnu.so on Linux, .cpython-313-darwin.so on macOS, .pyd on Windows). We need it to correctly identify compiled extension modules vs other files in the install directory — a plain ".so" check would miss the full CPython-tagged suffixes.
There was a problem hiding this comment.
I'll just try copying everything but .pyi files as this really is unnecessarily complicaated. Sorry.
python/CMakeLists.txt
Outdated
| if(DEFINED SKBUILD_STATE | ||
| AND SKBUILD_STATE STREQUAL "wheel" | ||
| AND NOT CMAKE_SYSTEM_NAME STREQUAL "Emscripten" | ||
| AND DEFINED ENV{CI} |
There was a problem hiding this comment.
If I understand this correctly this will default to ON on wheels and only on CI, otherwise is off and we have to turn it on manually, i.e. testing locally?
Is this logic necessary? Should we just remove all that, add the option and enable it as part of the wheel build scripts by setting an env var? We can add PYARROW_REQUIRE_STUB_DOCSTRINGS as a possible option on our pyproject.toml:
PYARROW_REQUIRE_STUB_DOCSTRINGS = {env = "PYARROW_REQUIRE_STUB_DOCSTRINGS", default = "OFF"} similar to PYARROW_BUNDLE_ARROW_CPP
There was a problem hiding this comment.
Ok, let me try that. Do we want to enable PYARROW_REQUIRE_STUB_DOCSTRINGS at dev time?
| def _create_importable_pyarrow(pyarrow_pkg, source_dir, install_pyarrow_dir): | ||
| """ | ||
| stubs_dir, build_lib = Path(stubs_dir), Path(build_lib) | ||
| Populate pyarrow_pkg with source Python modules and installed binary artifacts |
There was a problem hiding this comment.
What does Populate pyarrow_pkg mean? I am not sure I understand what is pyarrow_pkg here. Is it the source tree? Are we copying the Arrow shared objects around to populate the stubs? Otherwise I am unsure what are we copying/linking. Isn't this what PYARROW_BUNDLE_ARROW_CPP does? Maybe we have to build after CMake runs the copy for PYARROW_BUNDLE_ARROW_CPP?
There was a problem hiding this comment.
The idea is that pyarrow_pkg is a folder where we can import pyarrow from, so we can grab runtime docstrings from it. Updating comment to make it clearer.
Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>
|
Thanks for the review @raulcd, I've pushed some changes, can you check if this now makes sense? |
|
@kou could you do a pass here too? Especially feedback on CMake changes would be valuable :). |
5576c51 to
e7e51db
Compare
fab8f56 to
a1b43d8
Compare
Rationale for this change
Warning: should not be merged before #49259.
See #49452 and #48618
What changes are included in this PR?
Adds a wheel build time script to populate stubfiles with runtime docstrings.
Are these changes tested?
Not yet.
Are there any user-facing changes?
Users will get docstrings.